Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(s2n-quic-dc): handle spurious TCP acceptor worker wakeups #2434

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Jan 2, 2025

Description of changes:

This change fixes an issue where the following happens:

  • The TCP acceptor removes a stream at index N, due to it being expired
  • The tokio runtime is notified of stream readiness at index N and wakes the task
  • The task polls the worker, which is no longer reading from the stream, which was previously assumed to not happen

Testing:

I've made the fuzz tests less constrained to trigger this series of events. If I simply apply the test change, then it shows the error:

---- stream::server::tokio::tcp::manager::tests::invariants_test stdout ----

======================== Test Failure ========================

BOLERO_RANDOM_SEED=148028850864596720821768053785163681304

Input: 
[
    Wake {
        idx: 1,
    },
    Insert,
]

Error: 
panicked at dc/s2n-quic-dc/src/stream/server/tokio/tcp/manager/tests.rs:94:17:
internal error: entered unreachable code: shouldn't be polled when idle
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft camshaft marked this pull request as ready for review January 2, 2025 23:12
@camshaft camshaft enabled auto-merge (squash) January 2, 2025 23:13
@camshaft camshaft merged commit ac52a48 into main Jan 2, 2025
129 checks passed
@camshaft camshaft deleted the camshaft/dc-accept-spurious-wake branch January 2, 2025 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants